Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Street geo #516

Merged
merged 24 commits into from
May 21, 2024
Merged

Street geo #516

merged 24 commits into from
May 21, 2024

Conversation

Algorush
Copy link
Collaborator

No description provided.

Copy link

netlify bot commented May 15, 2024

Deploy Preview for 3dstreet-core-builds ready!

Name Link
🔨 Latest commit 6501ff3
🔍 Latest deploy log https://app.netlify.com/sites/3dstreet-core-builds/deploys/664cc851b1ce8500086317b4
😎 Deploy Preview https://deploy-preview-516--3dstreet-core-builds.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Algorush
Copy link
Collaborator Author

Algorush commented May 16, 2024

I have changed this example for testing: /examples/google-tiles/
Now you can change the component parameters like this in console:

r = document.getElementById('reference-layers')
r.setAttribute('street-geo', 'maps', 'mapbox2d')
r.setAttribute('street-geo', 'maps', 'google3d')
r.setAttribute('street-geo', 'maps', 'google3d, mapbox2d')
r.setAttribute('street-geo', 'maps', '')

The lat/long change still needs to be tested

// <mapName> : <function that creates and return map element>
'mapbox2d': this.createMapbox2dElement.bind(this),
'google3d': this.createGoogle3dElement.bind(this)
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mapTypes could be defined in init not update

@Algorush
Copy link
Collaborator Author

I renamed the map creation functions. Now the function has a name: MapType + "Create". Similar to the update functions: MapType + "Update".
I don't know, maybe it's not good practice to do like this?

@kfarr
Copy link
Collaborator

kfarr commented May 16, 2024

Elevation is the requested elevation of the scene's origin 0 0 0. In other words, if a user has lat 30, long 30, elevation 30, then I assume the elevation is 30 meters above sea level.

But google 3d tiles takes height which is a different value. Here is a sample of how I converted elevation into height in a glitch prototype project:

          // Default is SF Fort Mason Northwest Pier Streetlight
          const demoLat = queryParams.get("lat") ?? 37.807193;
          const demoLong = queryParams.get("long") ?? -122.431964;
          const elevationHeightConstant = 32.49158 // a "height" of -26 = 6.49m elevation therefore 32.49158
          const demoHeight = queryParams.get("elevation") - elevationHeightConstant ?? -26
          console.log(demoHeight);
          console.log(demoLat, demoLong);
          this.el.setAttribute('loader-3dtiles', 'long', demoLong)
          this.el.setAttribute('loader-3dtiles', 'lat', demoLat)
          this.el.setAttribute('loader-3dtiles', 'height', demoHeight)


const mapbox2dElement = document.createElement('a-entity');
mapbox2dElement.setAttribute('data-layer-name', 'Mapbox Satellite Streets');
mapbox2dElement.setAttribute('geometry', 'primitive: plane; width: 512; height: 512;');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a mix of spaces and tabs in the file, be sure to replace all tabs by spaces.

});

},
removeChildMaps: function () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be used.

@kfarr
Copy link
Collaborator

kfarr commented May 17, 2024

Next:

  • documentation for the component
  • test - does it create a child with the proper component properties? does it update the child when the wrapper properties are updated? (coordinates or map type)

@Algorush
Copy link
Collaborator Author

Algorush commented May 18, 2024

added documentation and test based on chai lib. Test could be run from /test/browserTests/street-geo-test.html
Chai library supports execution through the browser, so it is more convenient to use for testing when working with DOM, AFRAME and Three.js elements.

@Algorush
Copy link
Collaborator Author

Algorush commented May 18, 2024

I also added a .node.js ending to all existing tests, so that only tests with a .node.js ending would be run in test command through package.json.
For now, browser tests need to be run manually. Perhaps they can also be configured to launch via gitHub CI.
... yes, chatGPT suggests that browser tests can be run in Github Actions via puppeter or playwright

expect(mapbox2dElement).to.exist;
expect(google3dElement).to.exist;
done();
}, 1000);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need to wait 1s? We don't have much tests so that may be fine but if we can avoid that it would be better. You probably just need two setTimeout to be sure the street-geo is initialized and the child entity with components are created and initialized.

setTimeout(() => {
   setTimeout(() => {
     const mapbox2dElement = el.querySelector('[data-layer-name="Mapbox Satellite Streets"]');
      const google3dElement = el.querySelector('[data-layer-name="Google 3D Tiles"]');
      expect(mapbox2dElement).to.exist;
      expect(google3dElement).to.exist;
      done();
  });
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip!

@vincentfretin
Copy link
Collaborator

For tests, the main convention I saw in several projects is to mirror the directory structure and end the test filename with .spec.js or .test.js. The aframe repository for example uses .test.js convention.
So like this:

3dstreet/
├── src/
│   ├── components/
│   │   └── street-geo.js
├── test/
│   ├── components/
│   │   └── street-geo.test.js

@vincentfretin
Copy link
Collaborator

Those new tests should be able to be run with mocha and jsdom, no need to spawn a browser. I see existing tests were using jsdom-global already.

@Algorush
Copy link
Collaborator Author

Algorush commented May 18, 2024

Those new tests should be able to be run with mocha and jsdom, no need to spawn a browser. I see existing tests were using jsdom-global already.

Hi, existing tests in 3DStreet do not use AFRAME. I had difficulty to run AFRAME in Node.js environment. And I thought that running AFRAME in a browser environment with the chai library would allow to write tests more conveniently than in Node.js environment. I have now discovered information about using AFRAME in the Node.js environment. I'll try to do it this way

@Algorush
Copy link
Collaborator Author

Algorush commented May 19, 2024

I tried using only jsdom, mocha. I still can't run tests for the street-geo component that using AFRAME in a Node.js environment. I was using this info in AFRAME docs about running AFRAME in Node.js, but I'm getting errors in the console when running tests:

import * as SUPER_THREE from 'super-three';
^^^^^^

SyntaxError: Cannot use import statement outside a module

that I haven't found a way to fix. I see these options so far:

  1. I can make a simplified version of the street-geo component for the test by disabling the parts of the code that access AFRAME. Something like a test mode without AFRAME functions. If we only test adding/removing entities.
  2. Use a testing environment similar to the one used when testing AFRAME: https://github.com/aframevr/aframe/tree/master/tests
  3. Use the tests in chai browser environment, perhaps it will be easier than option 2: chai + mocha + puppeter for github Actions (it’s not difficult to set up)

Another option is to further try to fix the errors in point 1, that is, change the three.js code referenced by Aframe. I already had this experience a year ago when I adapted Three.js to work on the server side. But I think it would be easier to do 2 or 3

@vincentfretin
Copy link
Collaborator

The aframe doc is not be up to date, using require('aframe/src'); only works in a webpack environment like karma-webpack because of aframe/src/lib/three.module.js that's the only code where it uses ES module and also the example in the aframe repo with npm run test:node is currently broken. Now that node 22 supports requiring ES module with require("somemodule.mjs") see https://nodejs.org/en/blog/announcements/v22-release-announce#support-requireing-synchronous-esm-graphs,
I made an attempt to fix that in aframe repo in https://github.com/vincentfretin/aframe/tree/test-node and actually it works, but this can only tests basic functions because node environment lacks customElements, even with jsdom.
So yeah long story short, you still need to spawn a browser via karma for example like aframe tests are using.

@vincentfretin
Copy link
Collaborator

So yeah for now executing the tests manually by accessing
http://192.168.1.15:7001/test/browserTests/street-geo-test.html
that is executing test/browserTests/street-geo-test-bdd.mjs is enough. (see if you can use the setTimeout(setTimeout there too.)
and we can remove test/street-geo.test.js and aframe dependency.
We'll look at using karma and karma-webpack in another PR @kfarr I think.

@vincentfretin
Copy link
Collaborator

FYI I made this PR aframevr/aframe#5522

@kfarr kfarr merged commit 913a0ef into main May 21, 2024
3 of 5 checks passed
@kfarr kfarr deleted the street-geo branch May 21, 2024 16:14
@kfarr kfarr restored the street-geo branch May 21, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants